Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add: DockerHub で提供されているイメージに arm 版を追加 #639

Merged
merged 4 commits into from
Mar 29, 2023
Merged

add: DockerHub で提供されているイメージに arm 版を追加 #639

merged 4 commits into from
Mar 29, 2023
+58 −19

Conversation

K-shir0
Copy link
Contributor

@K-shir0 K-shir0 commented Mar 27, 2023 •
edited
Loading

内容

関連 Issue

close #633

スクリーンショット・動画など

image

その他

  • onnxruntime の arm64-gpu が見当たらなかった cpu のみ対応
  • GPU 環境がないので gpu 版のイメージが正しく動作するかの検証は出来ていない

@K-shir0 K-shir0 requested a review from a team as a code owner March 27, 2023 11:58
@K-shir0 K-shir0 requested review from y-chan and removed request for a team March 27, 2023 11:58
@K-shir0 K-shir0 changed the title add: arm64 support add: DockerHub image support for ARM architecture Mar 27, 2023
@K-shir0 K-shir0 changed the title add: DockerHub image support for ARM architecture add: DockerHub で提供されているイメージに arm 版を追加 Mar 27, 2023
@K-shir0
Copy link
Contributor Author

K-shir0 commented Mar 27, 2023 •
edited
Loading

アーキテクチャごとに workflow の build_args を切り替えることができればもっとスマートだったんでしたが、、TARGETPLATFORM を見て切り替える方法しか見つからなかったです。

Hiroshiba
Hiroshiba reviewed Mar 27, 2023
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

おーーーー　docker buildにplatformsを指定でき、しかもそれを同じタグにpushできることを知りませんでした！！！

ただこうするとdockerfile内でonnxruntime_urlを作る必要があり、バイナリビルドしている build.yml 側とコードがずれてしまうことに気づきました。
コードを同じような形にするには、１つのjob内で２プラットフォームビルドするのを、２つのjobで１つずつビルドしてpushできる必要がありそうなのですが、可能かご存知でしょうか･･･？

まあでも問題ではないと思うので、頂いたPRの方針で良いかも。
@y-chan さん的にどうでしょう 👀

@Hiroshiba
Copy link
Member

あ！　すでに質問の部分は答えてくださってました、すみません！
であればarm64ビルドの配布を優先し、今のPRの形で進むのが良いのかなとか思いました！

@github-actions
Copy link

github-actions bot commented Mar 27, 2023 •
edited
Loading

Coverage Result

Resultを開く
Name Stmts Miss Cover
voicevox_engine/init.py 1 0 coverage-100%
voicevox_engine/acoustic_feature_extractor.py 75 0 coverage-100%
voicevox_engine/dev/synthesis_engine/init.py 2 0 coverage-100%
voicevox_engine/dev/synthesis_engine/mock.py 36 2 coverage-94%
voicevox_engine/full_context_label.py 162 3 coverage-98%
voicevox_engine/kana_parser.py 86 1 coverage-99%
voicevox_engine/metas/Metas.py 33 0 coverage-100%
voicevox_engine/metas/MetasStore.py 29 14 coverage-52%
voicevox_engine/metas/init.py 2 0 coverage-100%
voicevox_engine/model.py 150 9 coverage-94%
voicevox_engine/mora_list.py 4 0 coverage-100%
voicevox_engine/part_of_speech_data.py 5 0 coverage-100%
voicevox_engine/preset/Preset.py 12 0 coverage-100%
voicevox_engine/preset/PresetError.py 2 0 coverage-100%
voicevox_engine/preset/PresetManager.py 81 2 coverage-98%
voicevox_engine/preset/init.py 4 0 coverage-100%
voicevox_engine/setting/Setting.py 11 0 coverage-100%
voicevox_engine/setting/SettingLoader.py 19 0 coverage-100%
voicevox_engine/setting/init.py 3 0 coverage-100%
voicevox_engine/synthesis_engine/init.py 5 0 coverage-100%
voicevox_engine/synthesis_engine/core_wrapper.py 201 159 coverage-21%
voicevox_engine/synthesis_engine/make_synthesis_engines.py 57 49 coverage-14%
voicevox_engine/synthesis_engine/synthesis_engine.py 130 11 coverage-92%
voicevox_engine/synthesis_engine/synthesis_engine_base.py 67 9 coverage-87%
voicevox_engine/user_dict.py 144 11 coverage-92%
voicevox_engine/utility/init.py 4 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 37 0 coverage-100%
voicevox_engine/utility/mutex_utility.py 10 0 coverage-100%
voicevox_engine/utility/path_utility.py 26 8 coverage-69%
TOTAL 1398 278 coverage-80%

y-chan
y-chan approved these changes Mar 28, 2023
View reviewed changes
Copy link
Member

@y-chan y-chan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

現在のPR方針で問題ないと思います...!
特に問題も見当たらなさそうだったので、LGTMとしたいと思います！

@K-shir0
Copy link
Contributor Author

K-shir0 commented Mar 28, 2023 •
edited
Loading

@Hiroshiba @y-chan

コードを同じような形にするには、１つのjob内で２プラットフォームビルドするのを、２つのjobで１つずつビルドしてpushできる必要がありそうなのですが、可能かご存知でしょうか･･･？

コードを同じようにかつ 2つ job(arm64, amd64) をくっつけるという方針のパターンを一つ考えてみました。
こちらの方が URL を Dockerfile で組み立てる必要がないですし、Dockerfile を変更する必要がないです。

検証 PR
K-shir0#2

job が分かれていい感じになりますが、デメリットもあり dockerhub 側で 〇〇-arm64 〇〇-amd64 というイメージが置かれてしまうところです。（中間イメージ的なの）

前の方針で良いとかあれば全然 Revert します

Hiroshiba
Hiroshiba reviewed Mar 28, 2023
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

おーー　manifestってこういう機能用だったんですね！！
とても勉強になります！！！

頑健な一方、キャッシュを作る必要があるといった課題もあるのかなという感想です。
（まあもともとbuildcacheがありますが）

利点欠点並べてみます。

うーーーーーん！！
すごく難しい判断でどちらも素晴らしいのですが、利点が多そうな前回の方法が良いのかなと思いました･･･ 🙇‍♂️

せっかくPR頂いたのでいろいろコメントしていますが、revertをお願いしてもよろしいでしょうか。。
将来、今のPRの方針でいくこのになった場合の参考になるので、resetではなくrevertだととても嬉しいです…🙇‍♂️

ほんとに申し訳ないです🙇‍♂️

.github/workflows/build-docker.yml Outdated
base_image: ubuntu:18.04
base_runtime_image: ubuntu:18.04
voicevox_core_asset_prefix: voicevox_core-linux-x64-cpu
onnxruntime_url: https://github.com/microsoft/onnxruntime/releases/download/v1.14.1/onnxruntime-linux-aarch64-1.14.1.tgz
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

バージョン合わせておくと環境ごとの差異なくせて良いかも？

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

完全に僕の転記ミスです🙇‍♂️

.github/workflows/build-docker.yml Outdated
target: runtime-env
base_image: ubuntu:18.04
base_runtime_image: ubuntu:18.04
voicevox_core_asset_prefix: voicevox_core-linux-x64-cpu
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arm64かもです

.github/workflows/build-docker.yml Outdated
target: runtime-env
base_image: ubuntu:20.04
base_runtime_image: ubuntu:20.04
voicevox_core_asset_prefix: voicevox_core-linux-x64-cpu
onnxruntime_url: https://github.com/microsoft/onnxruntime/releases/download/v1.13.1/onnxruntime-linux-x64-1.13.1.tgz
platforms: linux/amd64
- tag: cpu
tag_platform_prefix: arm64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

最後につけてるのでpostfixかも。
あと「プットフォームのタグのpostfix」とすると意味が通りやすいので、platform tag postfixにすると他と揃うかも…？（微妙かも……）

@Hiroshiba
Copy link
Member

あとそういえばnvidia+arm64のペアが省かれていますが、これは基本になるdocker imageがないとかでしょうか 👀

@K-shir0
Copy link
Contributor Author

K-shir0 commented Mar 28, 2023

あとそういえばnvidia+arm64のペアが省かれていますが、これは基本になるdocker imageがないとかでしょうか 👀

arm 版 onnxruntime リリースが見当たらなかったためです、ドキュメントを確認してもはっきり動くと書いていませんでした。
さらに pip install onnxruntime-gpu でインストールできるパターンがあると知りどのようなファイルが降ってくるか確認したところここでも aarch64-gpu がなかったのでないのかなと判断しました。ここら辺の知見に乏しいのでなにか補足とかあれば頂きたいです。

https://pypi.org/project/onnxruntime-gpu/#files

@K-shir0
Copy link
Contributor Author

K-shir0 commented Mar 28, 2023

レビューありがとうございました！！ご指摘の通り、revert しました。

頑健な一方、キャッシュを作る必要があるといった課題もあるのかなという感想です。

manifest に関してはテキストの情報（josn）をくっつけているだけっぽくなので特にキャッシュをしなくても速いといった感想です。(下記画像で37s)

image

@K-shir0
Copy link
Contributor Author

K-shir0 commented Mar 29, 2023

一応関連っぽいのでコメント #322

Hiroshiba
Hiroshiba approved these changes Mar 29, 2023
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM！！

revertすみません、ありがとうございます🙇‍♂️

arm64とcudaが無いのはonnxruntime側でしたか！！
把握できてませんでした、なるほどです。

たしかにmanifest作成はたしかにめちゃめちゃ早そうですね！！
アーキテクチャごとにimageがpushされて、一時ファイルが残るため検索性が落ちちゃうのを気にしていました。
Actions側でimageをまとめてからpushするとかできそうですが、それならplatformで判定する今の形のが綺麗だよなーと…！

@Hiroshiba
Copy link
Member

マージします！
自動でlatestがdockerビルドされるはずです。

PRありがとうございました、もしよければまたお待ちしています！！
例えばビルド周りでしたら他にもこういうのがあったりします。ご興味あれば…！！

@Hiroshiba Hiroshiba merged commit 3478a28 into VOICEVOX:master Mar 29, 2023
@aoirint aoirint mentioned this pull request Apr 24, 2023
Closed
@aoirint aoirint mentioned this pull request May 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@y-chan y-chan y-chan approved these changes

@Hiroshiba Hiroshiba Hiroshiba approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mac M1ではDockerが動作しない
3 participants
@K-shir0 @Hiroshiba @y-chan